Video Controls Overlay #838
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
netpro2k
force-pushed
the
feature/video-overlays
branch
from
a4cd041
to
309b711
Compare
| this.el.setAttribute("media-video", "volume", THREE.Math.clamp(this.data.volume + volumeMod, 0, 1)); | ||
| this.volumeLabel.setAttribute( |
There was a problem hiding this comment.
You might avoid some work with:
this.volumeLabel.components.text.updateGeometry(this.geometry, font);
this.volumeLabel.components.text.updateLayout();
We have reservations for setAttribute and also the update for the text component does some checks that aren't necessary for you (because you aren't changing font or shader or texture here.
There was a problem hiding this comment.
This would leave the components data in a weird state. Since this is only when adjusting volume I am not too worried about it.
| @@ -388,6 +376,11 @@ AFRAME.registerComponent("media-video", { | |||
| this.video.removeEventListener("pause", this.onPauseStateChange); | |||
| this.video.removeEventListener("play", this.onPauseStateChange); | |||
| } | |||
| if (this.hoverMenu) { | |||
There was a problem hiding this comment.
Might not exist if somehow this remove is called before the hoverMenu promise resolves
| this.volumeLabel.object3D.visible = true; | ||
| clearTimeout(this.hideVolumeLabelTimeout); | ||
| if (this.data.volume) { | ||
| this.hideVolumeLabelTimeout = setTimeout(() => (this.volumeLabel.object3D.visible = false), 1000); |
There was a problem hiding this comment.
Ok to set a timeout every frame? I suppose people aren't messing with volume all the time...
An alternative might be to write the time that the volume label should be made invisible here since tick is going to keep being called
There was a problem hiding this comment.
Yeah again, since this is only when manipulating volume I don't feel too worried about it.
src/components/media-views.js
Outdated
| this.seekForwardButton.object3D.visible = !!this.video && !this.videoIsLive; | ||
| this.seekBackButton.object3D.visible = !!this.video && !this.videoIsLive; | ||
| } | ||
|
|
||
| // Only update playback posiiton for videos you don't own |
There was a problem hiding this comment.
posiiton -> position
brianpeiris
added
whats new
This was referenced Aug 19, 2022
This was referenced Nov 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Move video playback controls into an overlay that is shown on hover instead of in the pause menu. Also add feedback for volume level and playback position.
This PR adds a component
hover-menuwhich can specify a template to show on hover of the entity (or any child entities). The menu is automatically hidden when paused to not conflict with the pause menu. Components wishing to use the hover menu can callgetMenu()on thehover-menucomponent to get a reference to the instantiated menu element (and then use the sub elements as they see fit).Note that the video overlay's layout currently assumes a 16:9 video. The controls will still work for other video sizes but will be positioned a bit oddly. Also the buttons are using multiple large textures similar to what we are doing in the HUD. This is not ideal but can be addressed as a whole when we decide to fix it in other parts of the UI. Also the way dimming works is pretty opinionated about the object the menu is on, we will need to expand this if we want to show hover menu on other objects.
This also fixes some bugs with volume control.